Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowing for multiple events of same name to be raised in parallel #194

Merged
merged 8 commits into from
Sep 27, 2023

Conversation

RyanLettieri
Copy link
Member

This change stems from the following issue discovered in dapr: dapr/dotnet-sdk#1129

eng/proto Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change here was intentional, was it?

src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs Outdated Show resolved Hide resolved
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link

@tmacam tmacam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but some minor fixes

src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs Outdated Show resolved Hide resolved
src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs Outdated Show resolved Hide resolved
Copy link
Member

@jviau jviau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the warnings before checking in.

@@ -14,7 +14,7 @@ namespace Microsoft.DurableTask.Worker.Shims;
/// </summary>
sealed partial class TaskOrchestrationContextWrapper : TaskOrchestrationContext
{
readonly Dictionary<string, IEventSource> externalEventSources = new(StringComparer.OrdinalIgnoreCase);
readonly Dictionary<string, Queue<IEventSource>> externalEventSources = new(StringComparer.OrdinalIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the typical event usage is, but I am going to hazard a guess single events are common. In which case allocating a queue for it is not optimal. We will have to revisit this and see if there is a way to optimize for a few element count cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed we should revisit this. It turns out we do the same thing with the NamedQueue<string> class for buffered events, so we should consider optimizing both.

@cgillum cgillum merged commit 0b44828 into microsoft:main Sep 27, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants